feat(node): use diagnostics_channel for redis >= 5.12.0#20573
feat(node): use diagnostics_channel for redis >= 5.12.0#20573
Conversation
There was a problem hiding this comment.
Pull request overview
Adds diagnostics_channel-based Redis tracing for upcoming node-redis (>= 5.12.0) while narrowing the existing IITM patching to avoid double instrumentation once diagnostics events are available.
Changes:
- Restrict vendored node-redis IITM patcher to
>=5.0.0 <5.12.0. - Add a diagnostics_channel subscriber which creates Sentry spans from
node-redis:*tracing channels. - Ensure
@sentry/opentelemetry/*subpath imports stay external in the Node rollup build.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/node/src/integrations/tracing/redis/vendored/redis-instrumentation.ts | Narrows the v5 IITM instrumentation version range to stop before 5.12.0. |
| packages/node/src/integrations/tracing/redis/redis-dc-subscriber.ts | New diagnostics_channel subscriber which creates/ends spans for command/batch/connect events and runs the responseHook. |
| packages/node/src/integrations/tracing/redis/index.ts | Wires the subscriber into Redis instrumentation (deferred to next tick). |
| packages/node/rollup.npm.config.mjs | Externalizes @sentry/opentelemetry/* imports so subpath entrypoints aren’t bundled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses | ||
| // `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager | ||
| // to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration | ||
| // `setupOnce`, so defer to the next tick. | ||
| Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook)); |
There was a problem hiding this comment.
The diagnostics_channel subscriber is deferred via a microtask. This means node-redis >=5.12.0 operations performed immediately after Sentry.init() (in the same synchronous tick) can run before the subscriber is attached and will not be traced. Consider hooking this up from the OpenTelemetry init path (after the context manager is registered) or otherwise ensuring subscription happens synchronously before user code continues.
| // node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses | |
| // `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager | |
| // to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration | |
| // `setupOnce`, so defer to the next tick. | |
| Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook)); | |
| // node-redis >= 5.12.0 publishes via diagnostics_channel. Subscribe synchronously so | |
| // operations performed immediately after `Sentry.init()` are traced without a race window. | |
| subscribeRedisDiagnosticChannels(cacheResponseHook); |
| // Test-only helper. | ||
| export function _resetRedisDiagnosticChannelsForTesting(): void { | ||
| subscribed = false; | ||
| currentResponseHook = undefined; | ||
| } |
There was a problem hiding this comment.
This file exposes _resetRedisDiagnosticChannelsForTesting(), but it only flips flags and does not unsubscribe any handlers already registered with diagnostics_channel. If tests call subscribeRedisDiagnosticChannels() again after a reset, you'll end up with duplicate subscribers (and duplicated spans). Consider keeping references to the subscriber objects (or channel instances) and calling channel.unsubscribe(...) during reset.
| * automatically via `bindStore` — without it, spans created in `start` would not become | ||
| * the active context for subsequent operations. | ||
| * | ||
| * Safe on every runtime that exposes `node:diagnostics_channel` (Node, Bun, Deno, Workers). |
There was a problem hiding this comment.
The JSDoc claims this is "Safe on every runtime" including Workers/Deno, but this is in @sentry/node (Node >=18) and relies on node:diagnostics_channel via @sentry/opentelemetry/tracing-channel. Consider tightening the wording to avoid implying support for runtimes where Node builtins (Buffer/diagnostics_channel) may not exist.
| * Safe on every runtime that exposes `node:diagnostics_channel` (Node, Bun, Deno, Workers). | |
| * Intended for Node environments where `node:diagnostics_channel` is available. | |
| * On runtimes without the required Node builtins, setup fails closed and no subscribers are registered. |
| const hook = currentResponseHook; | ||
| if (!hook) return; | ||
| try { | ||
| hook(span, command, args as unknown as Parameters<typeof hook>[2], result); |
There was a problem hiding this comment.
runResponseHook currently needs an unsafe cast for args to match the hook signature. Since CommandArgs already includes string | Buffer elements, this should be type-compatible without a cast (or you can type currentResponseHook as the redis response hook type). Removing the cast avoids masking real type mismatches.
| hook(span, command, args as unknown as Parameters<typeof hook>[2], result); | |
| hook(span, command, args, result); |
| // node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses | ||
| // `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager | ||
| // to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration | ||
| // `setupOnce`, so defer to the next tick. | ||
| Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook)); |
There was a problem hiding this comment.
This PR adds a new diagnostics_channel-based instrumentation path, but there are no unit tests covering span creation/end, error handling, or that the responseHook is invoked for command events. Since this package already has Redis tracing tests under packages/node/test/integrations/tracing/redis, it would be good to add a focused test suite for subscribeRedisDiagnosticChannels() using node:diagnostics_channel tracing channels.
size-limit report 📦
|
d150996 to
de731ed
Compare
node-redis 5.12.0 publishes command, batch, and connect events via node:diagnostics_channel. Subscribe to those channels via @sentry/opentelemetry/tracing-channel to produce spans without IITM- based monkey-patching, which lets the redis integration work on runtimes that don't support IITM (Bun, Deno, Cloudflare Workers). The existing OTel patcher is narrowed to '<5.12.0' so it does not double-instrument when both paths are present. The DC subscription is deferred to the next microtask so it runs after initOpenTelemetry() sets up the Sentry context manager (required for bindStore).
fdbf2b6 to
75744f4
Compare
75744f4 to
22acdcd
Compare
| const CHANNEL_BATCH = 'node-redis:batch'; | ||
| const CHANNEL_CONNECT = 'node-redis:connect'; | ||
|
|
||
| const ORIGIN = 'auto.db.redis.diagnostic-channel'; |
There was a problem hiding this comment.
Bug: batch and connect spans from the diagnostics_channel path have an inconsistent sentry.origin value because their asyncEnd handlers don't call runResponseHook to normalize it.
Severity: LOW
Suggested Fix
Call runResponseHook within the asyncEnd handlers for setupBatchChannel and setupConnectChannel, similar to how it's done in setupCommandChannel. This will ensure all spans created by the diagnostics_channel integration have a consistent sentry.origin attribute.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/redis/redis-dc-subscriber.ts#L25
Potential issue: Spans created for Redis `batch` and `connect` operations via the
`diagnostics_channel` path are assigned a `sentry.origin` of
`'auto.db.redis.diagnostic-channel'`. Unlike command spans, which have their origin
overwritten to `'auto.db.otel.redis'` by the `cacheResponseHook`, the `asyncEnd`
handlers for `batch` and `connect` spans do not invoke this hook. This results in an
inconsistent `sentry.origin` attribute across different types of Redis spans originating
from the same integration, which can affect telemetry processing and filtering.
Did we get this right? 👍 / 👎 to inform future reviews.
22acdcd to
38623e1
Compare
38623e1 to
f88ffb0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f88ffb0. Configure here.
| // tracingChannel from @sentry/opentelemetry requires `node:diagnostics_channel`. | ||
| // On runtimes where it isn't available, fail closed. | ||
| } | ||
| } |
There was a problem hiding this comment.
Preload path permanently disables DC-based Redis instrumentation
High Severity
When instrumentRedis is called from the preload path (--import @sentry/node/preload), the deferred microtask fires before initOpenTelemetry() registers the SentryContextManager. The tracingChannel() helper fails to obtain the AsyncLocalStorage lookup (the OTel context manager is still the default noop), so channels are created without bindStore — meaning transformStart never runs and _sentrySpan is never set. The subscribed flag is then set to true, preventing re-subscription when Sentry.init() later calls instrumentRedis() a second time with the context manager properly available. Since the IITM patcher is now version-gated to <5.12.0, Redis >= 5.12.0 in the preload path gets no instrumentation at all.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f88ffb0. Configure here.


Builds on #20510 and adds tracing channels subscribers via
node:diagnostics_channel.The module patchers are narrowed to
>=5.0.0 <5.12.0while the subscriber path runs unconditionally. it will be inert in older releases anyways and will activate when version 5.12 publishes the events.Verified that it works on Node and Bun equally as well, while IITM fails on Bun.